Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix noFallthroughCasesInSwitch/jsx object is not extensible #9921

Merged
merged 2 commits into from
Oct 30, 2020
Merged

Fix noFallthroughCasesInSwitch/jsx object is not extensible #9921

merged 2 commits into from
Oct 30, 2020

Conversation

ryota-murakami
Copy link
Contributor

Relative Issues #9868 #9429

I fixed broken yarn start that cause react-scripts can't handle correctly noFallthroughCasesInSwitch and jsx compileOptions.

Oct-27-2020 16-12-58

Thank you for your decent review 😀

Copy link
Contributor

@mrmckeb mrmckeb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iansu @ianschmitz I've had this reported to me privately too, this seems like a change to the behaviour of ts.readConfigFile where the object is no longer extensible.

@mrmckeb mrmckeb closed this Oct 27, 2020
@mrmckeb
Copy link
Contributor

mrmckeb commented Oct 27, 2020

Re-running CI.

@mrmckeb mrmckeb reopened this Oct 27, 2020
@benneq
Copy link
Contributor

benneq commented Oct 27, 2020

@ryota-murakami Have you check the inner properties of compilerOptions? Those should be frozen, too. Though a simple appTsConfig.compilerOptions = { ...appTsConfig.compilerOptions }; might not work.

@ianschmitz ianschmitz added this to the 4.0.1 milestone Oct 28, 2020
@ryota-murakami
Copy link
Contributor Author

@benneq @KonstantinSimeonov I'm sorry for slow response.

Have you check the inner properties of compilerOptions? Those should be frozen, too. Though a simple appTsConfig.compilerOptions = { ...appTsConfig.compilerOptions }; might not work.

I've checked inner properties of compilerOptions are frozen.
Bellow screenshot showing Object.isFrozen(appTsConfig.compilerOptions.module) return true after destructured property of appTsConfig.compilerOptions on new Object.

appConfig

And I'll share video clip that recorded running bellow code without Error.

  if (appTsConfig.compilerOptions == null) {
    appTsConfig.compilerOptions = {};
    firstTimeSetup = true;
  } else {
    appTsConfig.compilerOptions = { ...appTsConfig.compilerOptions };
    for(const option in appTsConfig.compilerOptions) {
      if (!Object.isFrozen(option)) new Error('appTsConfig.compilerOptions option should be frozen')
    }
  }

https://www.dropbox.com/s/912pp3xudr9rn9s/Screen%20Recording%202020-10-30%20at%2021.40.54.mov?dl=0

@KonstantinSimeonov
Copy link
Contributor

KonstantinSimeonov commented Oct 30, 2020

@ryota-murakami

    for(const option in appTsConfig.compilerOptions) {
      if (!Object.isFrozen(option)) new Error('appTsConfig.compilerOptions option should be frozen')
    }

This is iterating over the keys of the object, which are strings and are always frozen and is not checking whether the properties are frozen or not 😛 . I think @benneq 's idea was to make sure that no crashes would occur if any of appTsConfig.compilerOptions's properties is frozen. Although I think that this is unlikely, you can safeguard against it by deep cloning the whole object.

Co-authored-by: Konstantin Simeonov <kon.simeonov@protonmail.com>
@ryota-murakami
Copy link
Contributor Author

@KonstantinSimeonov Thanks, that iterator is garbage 😅
And merge your suggested solution. I confirm that working.

But I don't understand why guys care about property frozen status?
This problem root is Object isn't extensible, co script couldn't add suggested config.
I think it doesn't matter frozen status 🤔

@ianschmitz ianschmitz changed the title 4.0.0 Fix noFallthroughCasesInSwitch/jsx object is not extensible err… Fix noFallthroughCasesInSwitch/jsx object is not extensible Oct 30, 2020
@ianschmitz ianschmitz merged commit 3a98ed1 into facebook:master Oct 30, 2020
@ianschmitz
Copy link
Contributor

Thanks folks!

@ryota-murakami
Copy link
Contributor Author

ryota-murakami commented Nov 1, 2020

@benneq @KonstantinSimeonov I got it why should we handle isFroze() result of each compileOptions.
If isFroze(someOption) === true and original config from reading files value is different from CRA suggested,
the script couldn't apply change to suggested value.

Thank you for your support about this PR!

@vegawong
Copy link

@ianschmitz should it make a new release version?

oklas added a commit to oklas/react-app-alias that referenced this pull request Nov 21, 2020
@cseas
Copy link

cseas commented Nov 22, 2020

Can we please have a release with this fix? The problem is occurring for a lot of people in #9868

blackarctic added a commit to blackarctic/create-react-app that referenced this pull request Nov 25, 2020
* Fix noFallthroughCasesInSwitch/jsx object is not extensible (facebook#9921)

Co-authored-by: Konstantin Simeonov <kon.simeonov@protonmail.com>

* Add logo license to README

* Remove trailing space in reportWebVitals.ts (facebook#10040)

* docs: add React Testing Library as a library requiring jsdom (facebook#10052)

Co-authored-by: Ian Schmitz <ianschmitz@gmail.com>

* Increase Workbox's maximumFileSizeToCacheInBytes (facebook#10048)

* Create FUNDING.yml

* replace inquirer with prompts (facebook#10083)

- remove `react-dev-utils/inquirer` public import

* Prepare 4.0.1 release

* Prepare 4.0.1 release

* Publish

 - cra-template-typescript@1.1.1
 - cra-template@1.1.1
 - create-react-app@4.0.1
 - react-dev-utils@11.0.1
 - react-scripts@4.0.1

Co-authored-by: Ryota Murakami <dojce1048@gmail.com>
Co-authored-by: Konstantin Simeonov <kon.simeonov@protonmail.com>
Co-authored-by: Ian Sutherland <ian@iansutherland.ca>
Co-authored-by: sho90 <aznecosann@gmail.com>
Co-authored-by: Anyul Rivas <anyulled@gmail.com>
Co-authored-by: Ian Schmitz <ianschmitz@gmail.com>
Co-authored-by: Jeffrey Posnick <jeffy@google.com>
Co-authored-by: Evan Bacon <baconbrix@gmail.com>
blackarctic added a commit to blackarctic/create-react-app that referenced this pull request Apr 29, 2021
* Fix noFallthroughCasesInSwitch/jsx object is not extensible (facebook#9921)

Co-authored-by: Konstantin Simeonov <kon.simeonov@protonmail.com>

* Add logo license to README

* Remove trailing space in reportWebVitals.ts (facebook#10040)

* docs: add React Testing Library as a library requiring jsdom (facebook#10052)

Co-authored-by: Ian Schmitz <ianschmitz@gmail.com>

* Increase Workbox's maximumFileSizeToCacheInBytes (facebook#10048)

* Create FUNDING.yml

* replace inquirer with prompts (facebook#10083)

- remove `react-dev-utils/inquirer` public import

* Prepare 4.0.1 release

* Prepare 4.0.1 release

* Publish

 - cra-template-typescript@1.1.1
 - cra-template@1.1.1
 - create-react-app@4.0.1
 - react-dev-utils@11.0.1
 - react-scripts@4.0.1

* chore: bump web-vital dependency version (facebook#10143)

* chore: bump typescript version (facebook#10141)

Co-authored-by: Ian Schmitz <ianschmitz@gmail.com>

* Add TypeScript 4.x as peerDependency to react-scripts(facebook#9964)

* remove chalk from formatWebpackMessages (facebook#10198)

* Upgrade @svgr/webpack to fix build error (facebook#10213)

Co-authored-by: Ian Schmitz <ianschmitz@gmail.com>

* Improve vendor chunk names in development (facebook#9569)

* Update postcss packages (facebook#10003)

Co-authored-by: Ian Schmitz <ianschmitz@gmail.com>

* Recovered some integration tests (facebook#10091)

* Upgrade sass-loader (facebook#9988)

* Move ESLint cache file into node_modules (facebook#9977)

Co-authored-by: Ian Schmitz <ianschmitz@gmail.com>

* Revert "Update postcss packages" (facebook#10216)

This reverts commit 580ed5d.

* Remove references to Node 8 (facebook#10214)

* fix(react-scripts): add missing peer dependency react and update react-refresh-webpack-plugin (facebook#9872)

* Update using-the-public-folder.md (facebook#10314)

Some library --> Some libraries

* docs: add missing override options for Jest config (facebook#9473)

* Fix CI tests (facebook#10217)

* appTsConfig immutability handling by immer (facebook#10027)

Co-authored-by: mad-jose <joset@yeswearemad.com>

* Add support for new BUILD_PATH advanced configuration variable (facebook#8986)

* Add opt-out for eslint-webpack-plugin (facebook#10170)

* Prepare 4.0.2 release

* Publish

 - cra-template-typescript@1.1.2
 - cra-template@1.1.2
 - create-react-app@4.0.2
 - react-dev-utils@11.0.2
 - react-error-overlay@6.0.9
 - react-scripts@4.0.2

* tests: update test case to match the description (facebook#10384)

* Bump webpack-dev-server 3.11.0 -> 3.11.1 (facebook#10312)

Resolves facebook#10084 security vulnerability in websocket-driver library version 0.5.6, imported transitively by sockjs

* Upgrade eslint-webpack-plugin to fix opt-out flag (facebook#10590)

* update immer to 8.0.1 to address vulnerability (facebook#10412)

Resolves facebook#10411

Bumps immer version to 8.0.1 to address the prototype pollution
vulnerability with the current 7.0.9 version.

* Prepare 4.0.3 release

* Update CHANGELOG

* Publish

 - create-react-app@4.0.3
 - react-dev-utils@11.0.3
 - react-scripts@4.0.3

Co-authored-by: Ryota Murakami <dojce1048@gmail.com>
Co-authored-by: Konstantin Simeonov <kon.simeonov@protonmail.com>
Co-authored-by: Ian Sutherland <ian@iansutherland.ca>
Co-authored-by: sho90 <aznecosann@gmail.com>
Co-authored-by: Anyul Rivas <anyulled@gmail.com>
Co-authored-by: Ian Schmitz <ianschmitz@gmail.com>
Co-authored-by: Jeffrey Posnick <jeffy@google.com>
Co-authored-by: Evan Bacon <baconbrix@gmail.com>
Co-authored-by: Sahil Purav <sahil5684@gmail.com>
Co-authored-by: Hakjoon Sim <trainto@gmail.com>
Co-authored-by: Chris Shepherd <chris@chrisshepherd.me>
Co-authored-by: Jason Williams <936006+jasonwilliams@users.noreply.github.com>
Co-authored-by: Jabran Rafique⚡️ <jabranr@users.noreply.github.com>
Co-authored-by: John Ruble <johnruble@gmail.com>
Co-authored-by: Morten N.O. Nørgaard Henriksen <morten.n.o.henriksen@icloud.com>
Co-authored-by: Sergey Makarov <serega.s.makar@gmail.com>
Co-authored-by: EhsanKhaki <ehsankhfr@gmail.com>
Co-authored-by: Kristoffer K <merceyz@users.noreply.github.com>
Co-authored-by: Aviv Hadar <Avivhdr@gmail.com>
Co-authored-by: Tobias Büschel <13087421+tobiasbueschel@users.noreply.github.com>
Co-authored-by: mad-jose <44253495+josezone@users.noreply.github.com>
Co-authored-by: mad-jose <joset@yeswearemad.com>
Co-authored-by: Andrew Hyndman <ajhyndman@hotmail.com>
Co-authored-by: Brody McKee <mrmckeb@users.noreply.github.com>
Co-authored-by: James George <jamesgeorge998001@gmail.com>
Co-authored-by: Dion Woolley <woolley.dion@gmail.com>
Co-authored-by: Walker Clem <51654951+wclem4@users.noreply.github.com>
wombleton pushed a commit to AurorNZ/create-react-app that referenced this pull request Jun 1, 2021
…#9921)

Co-authored-by: Konstantin Simeonov <kon.simeonov@protonmail.com>
abhiisheek pushed a commit to abhiisheek/create-react-app that referenced this pull request May 19, 2023
…#9921)

Co-authored-by: Konstantin Simeonov <kon.simeonov@protonmail.com>
abhiisheek pushed a commit to abhiisheek/create-react-app that referenced this pull request May 24, 2023
…#9921)

Co-authored-by: Konstantin Simeonov <kon.simeonov@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants